Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR for interface class lookups #82209

Merged
merged 5 commits into from
Feb 23, 2023
Merged

JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR for interface class lookups #82209

merged 5 commits into from
Feb 23, 2023

Conversation

AndyAyersMS
Copy link
Member

@AndyAyersMS AndyAyersMS commented Feb 15, 2023 •
edited
Loading

The jit currently replaces calls to CORINFO_HELP_VIRTUAL_FUNC_PTR whose results are unused with null pointer checks.

But if we're looking up a method on an interface, this helper can now throw a variety of exceptions, so this optimization is incorrect.

Fixes #82127.

@AndyAyersMS
JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR
d5d98d7 
The jit currently replaces calls to `CORINFO_HELP_VIRTUAL_FUNC_PTR`
whose results are unused with null pointer checks.

But this helper can now throw a variety of exceptions, so this
optimization is incorrect.

Fixes dotnet#82127.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 15, 2023
@ghost ghost assigned AndyAyersMS Feb 15, 2023
@ghost
Copy link

ghost commented Feb 15, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak
See info in area-owners.md if you want to be subscribed.

Issue Details

The jit currently replaces calls to CORINFO_HELP_VIRTUAL_FUNC_PTR whose results are unused with null pointer checks.

But this helper can now throw a variety of exceptions, so this optimization is incorrect.

Fixes #82127.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 15, 2023 •
edited
Loading

@BruceForstall PTAL
cc @dotnet/jit-contrib

I could not figure out when or why the helper call behavior changed, but per SPMI this optimization only kicks in for this one test, so it's not that important.

[edit: initially thought the diff I saw was from the Main in reabstraction but realize now this is likely some other method. Tracking from SPMI method instances back to test cases is not always easy.]

@jkotas
Copy link
Member

jkotas commented Feb 16, 2023 •
edited
Loading

I have added this optimization on 2004/08/31 ... memories :-) It was based on customer feedback: Some (3rd party?) compiler used ldvirtftn + pop to implement null checks and the ask was to make it more efficient. Is this idiomatic pattern to implement null checks going to regress?

@AndyAyersMS
Copy link
Member Author

Is this idiomatic pattern to implement null checks going to regress?

Yes, if it is heavily used by some class of IL generators, we'll end up with helper calls instead of simple null checks, and perhaps miss downstream inferences that after this point the ref can't be null (that part I'd need to investigate further).

It seems like the possibility of some other exception kinds being thrown by CORINFO_HELP_VIRTUAL_FUNC_PTR is a relatively new thing -- I have not yet been able to track down exactly when or how this changed.

@jkotas
Copy link
Member

jkotas commented Feb 16, 2023

The interface method resolution can fail for number of different reasons. It has been the case since forever, we just did not notice. I think it should be still ok to do this optimization for non-interface methods.

@AndyAyersMS AndyAyersMS changed the title JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR JIT: don't optimize calls to CORINFO_HELP_VIRTUAL_FUNC_PTR for interface class lookups Feb 16, 2023
@AndyAyersMS
Copy link
Member Author

AndyAyersMS commented Feb 17, 2023 •
edited
Loading

Odd and seemingly unrelated failure on musl arm/arm64:

  Discovering: System.Collections.Immutable.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Collections.Immutable.Tests (found 2901 of 2986 test cases)
  Starting:    System.Collections.Immutable.Tests (parallel test collections = on, max threads = 2)

Assert failure(PID 22 [0x00000016], Thread: 41 [0x0029]): pImportSection == pModule->GetImportSectionForRVA(rva)
    File: /__w/1/s/src/coreclr/vm/prestub.cpp Line: 2991
    Image: /root/helix/work/correlation/dotnet

May be related, there are some diffs in arm64 linux crossgen2 to investigate.

@build-analysis build-analysis bot mentioned this pull request Feb 17, 2023
Open
5 tasks
@AndyAyersMS
Copy link
Member Author

I don't see those diffs building locally.

@AndyAyersMS
Copy link
Member Author

Going to update to latest main and retry.

This was referenced Feb 18, 2023
Closed
Open
@BruceForstall
Copy link
Member

Testing hit a lot of infra issues. Going to cycle it.

@BruceForstall BruceForstall reopened this Feb 19, 2023
@BruceForstall
Copy link
Member

/azp run runtime-jit-experimental

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS
Copy link
Member Author

Tizen armel: dotnet build tool installation crashed

Welcome to .NET 8.0!
---------------------
SDK Version: 8.0.100-alpha.1.23061.8

----------------
Installed an ASP.NET Core HTTPS development certificate.
To trust the certificate run 'dotnet dev-certs https --trust' (Windows and macOS only).
Learn about HTTPS: https://aka.ms/dotnet-https
----------------
Write your first app: https://aka.ms/dotnet-hello-world
Find out what's new: https://aka.ms/dotnet-whats-new
Explore documentation: https://aka.ms/dotnet-docs
Report issues and find source on GitHub: https://github.com/dotnet/core
Use 'dotnet --help' to see available commands or visit: https://aka.ms/dotnet-cli
--------------------------------------------------------------------------------------
/__w/1/s/eng/common/tools.sh: line 469:   547 Segmentation fault      (core dumped) "$_InitializeBuildTool" "$@"

@AndyAyersMS
Copy link
Member Author

This PR is jinxed.

@AndyAyersMS AndyAyersMS reopened this Feb 23, 2023
@AndyAyersMS
Copy link
Member Author

Failure to unzip build assets:

Searching for: **\libraries_bin_windows_x64_Release.zip under directory: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\
Found: 1 files to extract:
D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
Extracting file: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
D:\a\_work\_tasks\ExtractFiles_5e1e3830-fbfb-11e5-aab1-090c92bc4988\1.213.0\7zip\7z.exe x -oD:\a\_work\1\s\artifacts D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
ERROR: D:\a\_work\1\s\__download__\libraries_bin_windows_x64_Release\libraries_bin_windows_x64_Release.zip
Can not open the file as archive

@AndyAyersMS
Copy link
Member Author

Runtime failure looks like an instance of #25979.

@AndyAyersMS
Copy link
Member Author

SPMI replay failure is #82397.

@AndyAyersMS AndyAyersMS merged commit cbe05f9 into dotnet:main Feb 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BruceForstall BruceForstall BruceForstall approved these changes

Assignees

@AndyAyersMS AndyAyersMS

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test failure Loader/classloader/DefaultInterfaceMethods/reabstraction/reabstraction/reabstraction.sh
3 participants
@AndyAyersMS @jkotas @BruceForstall